@codex review

Workflow [PR], commit [51762a7]

Codex Review: Didn't find any major issues. You're on a roll.

Why there is an antalya-26.1 label? this is a PR to 25.8

Change make sense for 26.1 too, but good to approve that it fixed the issue before porting.

We're trying to abandon 25.8 :) We release it as is (hopefully, today/tomorrow), idk if we will do further releases

So I guess it is not a priority now (to verify something against 25.8, having in mind it will have to be re-verified later for 26.1)

Likely this was fixed in the branch. Let's try updating upon the latest changes

Audit Report: PR #1436

Low

• Iterator concurrency capability is not forwarded by wrapper iterators
  • Impact: PR #1436 lock-elision optimization can be silently disabled in wrapped iterator paths, so next() remains serialized under distributor mutex and latency improvement is lost.
  • Anchor: IObjectIterator wrappers (ObjectIteratorWithPathAndFileFilter, ObjectIteratorSplitByBuckets) + StorageObjectStorageStableTaskDistributor::getMatchingFileFromIterator.
  • Trigger: Iceberg iterator is wrapped before entering task distributor (filter or bucket-split paths).
  • Why this is a defect: The distributor checks iterator->has_concurrent_next(), but wrappers keep default false instead of forwarding to wrapped iterator, so behavior differs between wrapped and unwrapped equivalent flows.
  • Fix direction: Override and forward has_concurrent_next() in both wrapper classes.
  • Regression test direction: Assert wrapped IcebergIterator still reports has_concurrent_next()==true and follows unlocked path.

@@ -154,6 +154,19 @@ ObjectInfoPtr StorageObjectStorageStableTaskDistributor::getMatchingFileFromIter
     {
         ObjectInfoPtr object_info;

+        if (iterator->has_concurrent_next())
+        {
+            object_info = iterator->next(0);
+
+            if (!object_info)
+            {
+                LOG_TEST(log, "Iterator is exhausted");
+                std::lock_guard lock(mutex);
+                iterator_exhausted = true;
+                break;
+            }
+        }
+        else
         {
             std::lock_guard lock(mutex);
             object_info = iterator->next(0);

struct IObjectIterator
{
    virtual ~IObjectIterator() = default;
    virtual ObjectInfoPtr next(size_t) = 0;
    virtual size_t estimatedKeysCount() = 0;
    virtual std::optional<UInt64> getSnapshotVersion() const { return std::nullopt; }
};

Audit Report: PR #1436 Metric Attribution

Scope: IcebergIteratorNextMicroseconds observability behavior between system.events and system.query_log.

Confirmed defect

Medium

• IcebergIteratorNextMicroseconds is not attributed to query-level ProfileEvents
  • Impact: query-level monitoring and troubleshooting are misleading. system.events shows non-zero iterator time while system.query_log.ProfileEvents['IcebergIteratorNextMicroseconds'] remains zero, hiding per-query cost.
  • Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergIterator.cpp, src/Interpreters/ProcessList.cpp.
  • Trigger: Iceberg query that uses IcebergIterator producer thread.
  • Why defect: the metric is incremented in work executed by a ThreadFromGlobalPool thread that is not attached to the query thread group. system.query_log snapshots thread-group counters only.
  • Smallest logical reproduction:
    1. Run Iceberg query that scans metadata/manifest files.
    2. Check system.events for IcebergIteratorNextMicroseconds -> increases.
    3. Check system.query_log sum for ProfileEvents['IcebergIteratorNextMicroseconds'] -> remains zero.
  • Likely fix direction:
    • Capture thread_group = CurrentThread::getGroup() before scheduling producer.
    • Attach producer thread with ThreadGroupSwitcher inside lambda.
    • Keep existing exception and queue semantics unchanged.
  • Regression test direction:
    • Integration test: run one Iceberg query, then verify corresponding system.query_log record has non-zero ProfileEvents['IcebergIteratorNextMicroseconds'] when work is non-trivial.
    • Keep global system.events delta check for sanity.
  • Blast radius: Iceberg query observability; affects profiling dashboards and query-level performance triage.

Evidence

Producer thread created without thread-group attachment:

producer_task.emplace(
    [this]()
    {
        while (!blocking_queue.isFinished())
        {
            std::optional<ManifestFileEntryPtr> entry;
            try
            {
                entry = data_files_iterator.next();

Query log takes counters from query thread group:

if (thread_group)
{
    res.memory_usage = thread_group->memory_tracker.get();
    res.peak_memory_usage = thread_group->memory_tracker.getPeak();

    if (get_profile_events)
        res.profile_counters = std::make_shared<ProfileEvents::Counters::Snapshot>(thread_group->performance_counters.getPartiallyAtomicSnapshot());
}

Known good pattern (attach background worker to query group):

thread = ThreadFromGlobalPool(
    [&, thread_group = DB::CurrentThread::getGroup()]
    {
        /// Attach to current query thread group, to be able to
        /// have query id in logs and metrics from scanDataFunc.
        DB::ThreadGroupSwitcher switcher(thread_group, DB::ThreadName::DATALAKE_TABLE_SNAPSHOT);
        scanDataFunc();
    });

Confidence

• Overall confidence: High
  • The discrepancy follows directly from counter ownership: global counters vs query thread-group counters.
  • Code path shows missing thread-group attachment in Iceberg producer thread.

PR #1436 CI Verification Report

MasterCI Results

84 success, 2 failures, 2 pending/running, 3 skipped (91 total)

Both MasterCI failures are known flaky tests unrelated to Iceberg iterator locking.

Regression Tests (clickhouse-regression)

126 success, 3 failure, 1 pending (130 total)

• swarms (Release): Pre-existing failure, already failing on parent commit 7159a04.
• swarms (Aarch64) and tiered_storage_local (Aarch64): Passed on parent but failed on merge commit. However, both are completely unrelated to the PR changes (Iceberg iterator locking). These are generic "Process completed with exit code 1" infrastructure-level failures with no specific test failure details. The swarms and tiered_storage suites do not test Iceberg iterator behavior.

Iceberg-Specific Regression Tests

All 4 Iceberg regression suites PASSED:

Other CI Failures